Trail of bits 018 #9674
Trail of bits 018 #9674
Conversation
helper/dhutil/dhutil.go
Outdated
| func DeriveSharedKey(secret, ourPublic, theirPublic []byte) ([]byte, error) { | ||
| // Derive the final key from the HKDF of the secret and public keys. | ||
|
|
||
| //These must be consistently ordered we're on "our" side of key negotiation or the other. |
There was a problem hiding this comment.
Can you explain a bit more (or link to some reading material) why the result of the comparison determines the order of arguments?
There was a problem hiding this comment.
Internally, HKDF hashes the secret and two public keys. If Alice and Bob are doing DH key exchange, Alice calculates:
HKDF(secret, A, B) since ourPublic is A.
Bob calculates HKDF(secret, B, A), since Bob's ours is B. That produces a different value. Now we only care that both public keys participate in the derivation, so simply sorting them so they are in a consistent numerical order (either one would do) arrives at an agreed value.
There was a problem hiding this comment.
Mind adding that as a comment?
|
Looks like there's already a test (TestCertWithNameEndToEnd) that exercises encrypted sink tokens, can you copy or extend it to cover the new option? |
Ah, I see you found the test since you modified it. Looking at the change, it seems that this is going to be a breaking change even without the new option? |
I'm hoping it's a non-breaking change unless you set derive_key to true. Am I wrong? You're right that I blind edited the unit tests always to derive. |
I don't know, can we add a test variant that doesn't derive to reassure me? And I don't know enough about the multiplication change to say whether this is warranted, but do we also want a test where the client uses the old multiplier curve25519.ScalarMult? |
|
For certain we don't need to worry about the difference between ScalarMult and X25519: "// Deprecated: when provided a low-order point, ScalarMult will set dst to all So they're mathematically equivalent. But I will add a test case without derived keys. |
| @@ -3,24 +3,24 @@ package agent | |||
| import ( | |||
| "context" | |||
| "encoding/pem" | |||
| "github.com/hashicorp/vault/api" | |||
There was a problem hiding this comment.
Import diff block here should belong down below right?
* master: Add a section to the MySQL secrets plugin docs about x509 (#9757) Update documentation for MySQL Secrets Engine (#9671) Conditionally overwrite TLS parameters for MySQL secrets engine (#9729) Correctly mark Cassandra as not supporting static roles (#9750) changelog++ pki: Allow to use not only one variable during templating in allowed_domains #8509 (#9498) agent/templates: update consul-template to v0.25.1 (#9626) Restoring the example policies for blocking sha1 (#9677) changelog++ changelog++ Document the new SSH signing algorithm option. (#9197) CHANGELOG-+ CHANGELOG++ Trail of bits 018 (#9674)
* TOB-018 remediation * Make key derivation an optional config flag, off by default, for backwards compatibility * Fix unit tests * Address some feedback * Set config on unit test * Fix another test failure * One more conf fail * Switch one of the test cases to not use a derive dkey * wip * comments
Trail of bits 018 (#9674)
* TOB-018 remediation * Make key derivation an optional config flag, off by default, for backwards compatibility * Fix unit tests * Address some feedback * Set config on unit test * Fix another test failure * One more conf fail * Switch one of the test cases to not use a derive dkey * wip * comments
This addresses two items from Trail-of-Bits 018. First, we use a different
scalar multiplication for the DH params that returns an error if a low-order
curve is used producing an all-zero key result.
Second, we introduce an optional config flag for Vault Agent, "derive_key",
which additionally applies the HKDF-SHA256 key derivation function to the
shared secret and both public keys.